Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#824 Automatically convert -1 in maximum-test-shards to the maximum shard amount #825

Conversation

adamfilipow92
Copy link
Contributor

@adamfilipow92 adamfilipow92 commented May 29, 2020

Fixes #824

Checklist

  • Unit tested
  • release_notes.md updated

Sorry, something went wrong.

@adamfilipow92 adamfilipow92 self-assigned this May 29, 2020
@codecov-commenter
Copy link

codecov-commenter commented May 29, 2020

Codecov Report

Merging #825 into master will increase coverage by 0.08%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #825      +/-   ##
============================================
+ Coverage     80.02%   80.11%   +0.08%     
  Complexity      674      674              
============================================
  Files           151      151              
  Lines          3084     3087       +3     
  Branches        442      443       +1     
============================================
+ Hits           2468     2473       +5     
+ Misses          337      336       -1     
+ Partials        279      278       -1     

@adamfilipow92 adamfilipow92 marked this pull request as ready for review May 29, 2020 12:29
@adamfilipow92 adamfilipow92 marked this pull request as draft May 29, 2020 13:31
@adamfilipow92 adamfilipow92 marked this pull request as ready for review May 29, 2020 14:16
@adamfilipow92 adamfilipow92 force-pushed the #824-convert-1-max-test-shards-to-the-maximum-shard-amount branch from c9df66d to 3c78771 Compare June 1, 2020 11:39
Copy link
Contributor

@pawelpasterz pawelpasterz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this implementations stands between two solutions and we should decide which way to proceed.
@bootstraponline I don't know if this is desired behavior but I think we should either 'fix' all incorrect input values or throw an error for all.
With this implementation we treat -1 with special care but how about 0, < -1 and > 50.
We could change input to the closest acceptable value and maybe print message to inform use that incorrect input was provided and how flank process it...

WDYT?

test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt Outdated Show resolved Hide resolved
test_runner/src/main/kotlin/ftl/args/IArgs.kt Outdated Show resolved Hide resolved
@bootstraponline
Copy link
Contributor

@bootstraponline I don't know if this is desired behavior but I think we should either 'fix' all incorrect input values or throw an error for all.
With this implementation we treat -1 with special care but how about 0, < -1 and > 50.
We could change input to the closest acceptable value and maybe print message to inform use that incorrect input was provided and how flank process it...

  • -1 is a special value that means use the maximum amount of shards. This is an important feature for backwards compatibility and also forwards compatibility when FTL increases the maximum shard count.
  • 0 is technically a valid shard amount (equal to disabling sharding). I'm fine if this throws an exception, no strong opinions here.
  • < -1 is invalid and should throw an exception
  • > 50 is invalid and should throw an exception

@pawelpasterz
Copy link
Contributor

I understand, thanks for response!

@adamfilipow92 adamfilipow92 force-pushed the #824-convert-1-max-test-shards-to-the-maximum-shard-amount branch from 4618476 to 966bec7 Compare June 2, 2020 13:19
@adamfilipow92 adamfilipow92 merged commit 56aed84 into master Jun 3, 2020
@adamfilipow92 adamfilipow92 deleted the #824-convert-1-max-test-shards-to-the-maximum-shard-amount branch June 3, 2020 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix max-test-shards: -1 to use the max allowed shards
6 participants